Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial commit for Find My Plane integration #29

Closed
wants to merge 28 commits into from
Closed

Initial commit for Find My Plane integration #29

wants to merge 28 commits into from

Conversation

hankhank10
Copy link

As discussed, please find attached an initial pull request for the Find My Plane integration.

I have tried to keep new code as much as possible in two new files so that I minimise the risk of breaking anything in your code:

  1. findmyplane_plugin.py - this does all of the actual interaction with the Find My Plane server
  2. findmyplane.js - this does the front end stuff

For efficiency purposes (ie avoiding additional HTTP requests) I have piggy backed three datapoints into the ui json response and pulled these in the JS as part of the general update.

From a UI perspective you will have a much better view on what actually works from your user's perspective - please feel free to edit as you see fit.

The one additional requirement here is installing requests but given this is a common library I didn't think that would be an issue. If you absolutely want to avoid more dependencies then I could rework this in urllib/urllib2 but it results in much uglier code, so I stuck with requests for now.

I look forward to your review and comments!

Mark

@mracko
Copy link
Owner

mracko commented Jan 30, 2021

Thanks Mark! I'll have a look at it some time next week and give you my feedback.

I don't think that adding Requests will be an issue. As long as I can compile it using pyinstaller without problems then I see no reason why it shouldn't be a required library.

@mracko
Copy link
Owner

mracko commented Feb 5, 2021

I've been busy finishing work on v1.5 this week, but I'll have a look at the code in the next couple of days. There will probably be some minor code conflicts that will need to be ironed out, but nothing complicated.

@hankhank10
Copy link
Author

Great, v1.5 looks really nice.

Loading current flight plan is very cool, I might have to look into whether it's possible to send that to the web as that would be very cool.

Let me know if anything doesn't make sense in my pull request or if I should change any of the code to fit in with v1.5

@hankhank10
Copy link
Author

Good morning @mracko

I was wondering whether you had had a chance to look at this or whether there is anything I can do to assist?

@mracko
Copy link
Owner

mracko commented Feb 23, 2021

Hi @hankhank10

I was finally able to test your FMP integration and I have to say that I like it. There are, however, a few things that need to be ironed out in my opinion before we integrate it with MCA:

  1. Needs to be updated to v1.5.1 (this is an obvious one)
  2. I was having issues when the MCA was open in multiple browsers. It seems that multiple clients are causing issues.
  3. Opening the FMP link didn't always work at first. Sometimes I had to refresh the page. This could be related to 2.
  4. I've set up a Home Screen button on Android to access the MCA so it runs in full screen. If you click the FMP link, then it opens in the same window. It would be great to force it to open in a regular browser to make it multi-task capable. This is also the case on iOS.
  5. I'd change the default value state to disconnected. I don't want to force users to share their position by default.
  6. I'd position the tab behind "Other".
  7. How about renaming the tab button to "FMP online" or "FMP" - just to keep it short.
  8. The width of the tab button needs fixing. See below:

image

Let me know your "comments to my comments". Once these issues are ironed out, I see no reason why it shouldn't be part of this app.

Thanks a lot for your work!

Martin

@hankhank10
Copy link
Author

Given there have been pretty extensive changes to the base repo on this to update to version 1.5.1 I thought it easier to refork a fresh version and make my changes there rather than manage version issues.

As such I have raised a fresh pull request here which uses the new version and addresses your comments above: #38

I will close this pull request now

@hankhank10 hankhank10 closed this Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants